Skip to content

feat: replace isLevelEnabled guards with Pino interpolation values #4679

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
natanasow wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: replace isLevelEnabled guards with Pino interpolation values #4679

natanasow wants to merge 2 commits into from
+399 −392

Conversation

@natanasow
Copy link
Member

@natanasow natanasow commented Dec 4, 2025 •
edited
Loading

Description

This PR implements a comprehensive logging performance optimization by replacing isLevelEnabled guards with Pino's interpolation values (printf-style formatting) across the codebase. The current implementation uses JavaScript string interpolation within conditional guards, which causes performance overhead by building log messages even when the corresponding log level is disabled.

Key improvements:

  • Performance: Eliminates string building overhead
  • Readability: Removes cluttered conditional logging statements, making code cleaner and more maintainable
  • Consistency: Standardizes logging approach using %s for string interpolation and %o for object interpolation
  • RPC Method Cleanup: Removes redundant trace logging

Related issue(s)

Fixes #4080

Testing Guide

Changes from original design (optional)

N/A

Additional work needed (optional)

N/A

Checklist

  • I've assigned an assignee to this PR and related issue(s) (if applicable)
  • I've assigned a label to this PR and related issue(s) (if applicable)
  • I've assigned a milestone to this PR and related issue(s) (if applicable)
  • I've updated documentation (code comments, README, etc. if applicable)
  • I've done sufficient testing (unit, integration, etc.)

@natanasow
chore: fix eslint
aaecccb 
Signed-off-by: nikolay <[email protected]>
@natanasow natanasow added this to the 0.74.0 milestone Dec 4, 2025
@natanasow natanasow self-assigned this Dec 4, 2025
@natanasow natanasow requested review from a team as code owners December 4, 2025 10:32
@natanasow natanasow added the enhancement New feature or request label Dec 4, 2025
@natanasow natanasow requested a review from acuarica December 4, 2025 10:32
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 4, 2025
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@natanasow natanasow mentioned this pull request Dec 4, 2025
Closed
5 tasks
@github-actions
Copy link

github-actions bot commented Dec 4, 2025 •
edited
Loading

Test Results

 20 files  ±0  273 suites  ±0   22m 19s ⏱️ -1s
794 tests ±0  790 ✅ +1  4 💤 ±0  0 ❌  - 1 
810 runs  ±0  806 ✅ +1  4 💤 ±0  0 ❌  - 1 

Results for commit 819aac1. ± Comparison against base commit 75f8196.

♻️ This comment has been updated with latest results.

jasuwienas
jasuwienas previously approved these changes Dec 4, 2025
View reviewed changes
Copy link
Contributor

@jasuwienas jasuwienas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's pretty straightforward! I wasn't sure whether we should use %s for an array, but it matches the current behavior where we inject the values directly into the string, so it's fine (${['']} === [''].toString()).

That said, these changes will heavily conflict with the work I did for the Redis refactor. (and which I didn't merge because of the newman issue): #4623

Perhaps we can leave the updates to RedisCache.ts out of this PR and handle them in a separate task? wdyt?

konstantinabl
konstantinabl previously approved these changes Dec 5, 2025
View reviewed changes
simzzz
simzzz previously approved these changes Dec 5, 2025
View reviewed changes
@natanasow natanasow dismissed stale reviews from simzzz, konstantinabl, and jasuwienas via 819aac1 December 8, 2025 08:03
@codecov
Copy link

codecov bot commented Dec 8, 2025

Codecov Report

❌ Patch coverage is 87.06897% with 45 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/relay/src/lib/clients/sdkClient.ts 72.72% 15 Missing ⚠️
packages/relay/src/lib/clients/mirrorNodeClient.ts 80.39% 10 Missing ⚠️
...lay/src/lib/clients/cache/redisCache/redisCache.ts 79.16% 5 Missing ⚠️
...ices/ethService/contractService/ContractService.ts 87.50% 5 Missing ⚠️
...rvices/ethService/accountService/AccountService.ts 63.63% 4 Missing ⚠️
...barLimiter/evmAddressHbarSpendingPlanRepository.ts 92.30% 1 Missing ⚠️
...hbarLimiter/ipAddressHbarSpendingPlanRepository.ts 75.00% 1 Missing ⚠️
packages/relay/src/lib/eth.ts 95.45% 1 Missing ⚠️
...c/lib/services/ethService/feeService/FeeService.ts 90.00% 1 Missing ⚠️
...services/rateLimiterService/RedisRateLimitStore.ts 88.88% 1 Missing ⚠️
... and 1 more 
@@            Coverage Diff             @@
##             main    #4679      +/-   ##
==========================================
+ Coverage   95.67%   95.78%   +0.11%     
==========================================
  Files         131      131              
  Lines       21028    21006      -22     
  Branches     1785     1720      -65     
==========================================
+ Hits        20118    20121       +3     
+ Misses        892      866      -26     
- Partials       18       19       +1
Flag Coverage Δ
config-service 98.83% <ø> (ø)
relay 92.33% <71.47%> (+0.98%) ⬆️
server 88.99% <ø> (ø)
ws-server 97.93% <96.55%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...kages/relay/src/lib/clients/cache/localLRUCache.ts 97.42% <100.00%> (-0.02%) ⬇️
...ay/src/lib/config/hbarSpendingPlanConfigService.ts 97.68% <100.00%> (+1.53%) ⬆️
...sitories/hbarLimiter/hbarSpendingPlanRepository.ts 100.00% <100.00%> (+2.40%) ⬆️
packages/relay/src/lib/debug.ts 99.22% <100.00%> (-0.16%) ⬇️
...es/relay/src/lib/dispatcher/rpcMethodDispatcher.ts 99.00% <100.00%> (ø)
packages/relay/src/lib/relay.ts 98.64% <100.00%> (ø)
...b/services/ethService/blockService/BlockService.ts 98.71% <100.00%> (+2.07%) ⬆️
...vices/ethService/ethFilterService/FilterService.ts 97.26% <100.00%> (ø)
...kages/ws-server/src/service/subscriptionService.ts 100.00% <100.00%> (ø)
...barLimiter/evmAddressHbarSpendingPlanRepository.ts 97.20% <92.30%> (-0.02%) ⬇️
... and 10 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@simzzz simzzz simzzz approved these changes

@jasuwienas jasuwienas jasuwienas left review comments

@konstantinabl konstantinabl konstantinabl left review comments

@acuarica acuarica Awaiting requested review from acuarica acuarica is a code owner automatically assigned from hiero-ledger/hiero-json-rpc-relay-maintainers

@quiet-node quiet-node Awaiting requested review from quiet-node

At least 2 approving reviews are required to merge this pull request.

Assignees

@natanasow natanasow

Labels

enhancement New feature or request

Projects

None yet

Milestone

0.74.0

Development

Successfully merging this pull request may close these issues.

Use "interpolation values" instead of string interpolation when logging

5 participants

@natanasow @jasuwienas @simzzz @konstantinabl